Erb template - #40
Conversation
| @erb_args = erb_args | ||
| end | ||
|
|
||
| def parse_sdf_document(sdf_file) |
There was a problem hiding this comment.
I think you should make a difference between erb and non-erb files, that is parse ERB only when the extension is .erb
There was a problem hiding this comment.
I told him to do it like this. If you mean for having an explicit error when a non-erb file is given to an ERBLoader, I feel this is overkill and it would painful in the Robot level to constantly juggle between loaders when the model file changes (I dont think you mean this, just getting it out there).
In the case you want to split the functionality between parse_sdf_document, or do a plain load directly as its done nowadays when the file does not have a .erb, my concern would be the flakiness of someone defining a model.sdf that IS an file with ERB variables on it without realizing, and then the syntax error when interpreting the SDF would be probably very noisy.
There was a problem hiding this comment.
I agree with @jhonasiv on this one, but in this case I would simply remove the base SDF::Loader class and keep only the ERBLoader.
In case we enforce the files to end with .erb then I suggest we keep both loaders and make them only handle their specific file extension
But anyway, I don't have a strong opinion on this, so I would happily go with any
There was a problem hiding this comment.
A part from this comment, the review was addressed. I didn't address this one as it seems there is no consensus yet, let me know what to do about this one
|
|
||
| module SDF | ||
| # class to load SDF and ERB templated SDF files | ||
| class ERBLoader < Loader |
There was a problem hiding this comment.
I'm missing the purpose or advantage of having separate loader and ERBLoader classes. This stuff is so simple, why not a single class ?
There was a problem hiding this comment.
If I remember correctly, @jhonasiv requested me to do it so we could enforce that when the SDF::Loader is configured only .sdf files are loaded
3 participants
Add SDF::Loader and SDF::ERBLoader and use loader in
load